Assertions return boolean #2586
Conversation
|
@novemberborn I have edited test cases only for pass(), fail(), is() and not(). |
There was a problem hiding this comment.
@adiSuper94 thanks for redoing the PR.
I've pushed two commits, one for some weird code I spotted, and then the other to return false if the messages are illegal.
I think notThrowsAsync() should also return a boolean, just because we can. It's only the throws() and throwsAsync() assertions that return the thrown error or undefined (which we haven't typed because it's annoying).
With regards to the tests, I appreciate that the assertion tests are rather complicated. I'm struggling to understand the changes you've made though. Could you elaborate on your approach? For these tests too I'd prefer if we could use tap's assertions to check whether an AVA assertion returned true or false.
We do have some setup in place now to use AVA to test AVA, but migrating the assertion tests to that should be separate.
|
@adiSuper94 Bump :) |
|
@sindresorhus I have been meaning to work on this for the past few weekends. Thanks for the reminder. |
adiSuper94
force-pushed
the
feat/assertions-return-bool
branch
from
b403d51 to
2081a9e
Compare
adiSuper94
force-pushed
the
feat/assertions-return-bool
branch
from
2081a9e to
3d3a40e
Compare
|
@novemberborn , sorry for the delay in response.
Thanks, I guess my editor is acting a little crazy.
And added returns for
The approach I took was rather crude. I have changed it. Let me know if it clear now. |
There was a problem hiding this comment.
Looks good @adiSuper94. Thanks for coming back to this.
I've added two suggestions to make the tests actually look for false and true values, rather than falsy/truthy. What do you think?
Would you mind updating the type definitions as well?
|
You might be able to cherry-pick the type definitions from #2601. |
|
@novemberborn , Sure. |
Co-authored-by: Mark Wubben <[email protected]>
Co-authored-by: Mark Wubben <[email protected]>
I accepted those, suggestions. It actually helped me improve the tests. Thanks!
cherry-picked the type definitions from #2601 |
There was a problem hiding this comment.
Thanks @adiSuper94. Couple more questions. And I appreciate these assertion tests are rather cumbersome!
| t.pass(); | ||
| } else { | ||
| lastFailure = new Error('Assertion failed'); |
There was a problem hiding this comment.
You shouldn't need to assign / override this.
| @@ -1047,6 +1057,7 @@ test('.throws()', gather(t => { | |||
| assertions.throws(() => { | |||
| throw new Error('foo'); | |||
| }); | |||
| return true; | |||
There was a problem hiding this comment.
Why add this return statement? Same below.
| fn(); | ||
| if (lastFailure) { | ||
| if (fn() === false && lastFailure) { | ||
| t.pass(); |
There was a problem hiding this comment.
Should this be something like:
const retval = fn();
if (lastFailure) {
t.false(retval)
}And something similar in passes below. Combining the variable check with the return value check makes it hard to know what's being tested.
| @@ -433,7 +509,7 @@ export interface CbExecutionContext<Context = unknown> extends ExecutionContext< | |||
| end(error?: any): void; | |||
| } | |||
|
|
|||
| export type ImplementationResult = PromiseLike<void> | Subscribable | void; | |||
| export type ImplementationResult = PromiseLike<void> | Subscribable | boolean | void; | |||
There was a problem hiding this comment.
This shouldn't change. It's the return type of the function you pass to test().
|
Closing in favor of #2696. |
Fixes #2455. Based on #2564.